Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bring back slice::ref_slice as slice::from_ref. #45306

Merged
merged 2 commits into from
Nov 2, 2017
Merged

Bring back slice::ref_slice as slice::from_ref. #45306

merged 2 commits into from
Nov 2, 2017

Conversation

whitequark
Copy link
Member

@whitequark whitequark commented Oct 15, 2017

These functions were deprecated and removed in 1.5, but such simple
functionality shouldn't require using unsafe code, and it isn't
cluttering libstd too much.

The original removal was quite contentious (see #27774), since then
we've had precedent for including such nuggets of functionality (see rust-lang/rfcs#1789),
and @nikomatsakis has provided a lot of use cases in rust-lang/rfcs#1789 (comment).
Hence this PR.

I'm not too sure what to do with stability, feel free to correct me.
It seems pointless to go through stabilization for these functions though.

cc @aturon

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@whitequark
Copy link
Member Author

I didn't do anything to bring back Option::as_slice and Result::as_slice because x.map_or(&[], ref_slice) is almost as short and just as easy to understand.

@kennytm kennytm added C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Oct 15, 2017
@ghost
Copy link

ghost commented Oct 15, 2017

Maybe it's just me, but, looking at the name alone, it's hard to tell what slice::ref_slice actually does.
Perhaps slice::from_ref would sound better? Or slice::single_ref? 🚲

@sfackler
Copy link
Member

I'm not particularly opposed to bringing this back, but I do agree with @stjepang's naming concern. We may want to land these as unstable initially, though I don't really know what good that would do in this specific case.

@rfcbot fcp merge

@Mark-Simulacrum Mark-Simulacrum added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Oct 18, 2017
@Mark-Simulacrum
Copy link
Member

I've added the waiting-on-crater label for now so that if we decide to go ahead with this we do a crater run -- I think it's worth doing that just in case others have implemented ref_slice (which could play into the naming bikeshed, too, so probably best after fcp).

@rfcbot
Copy link

rfcbot commented Oct 18, 2017

Team member @sfackler has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Oct 18, 2017
@sfackler
Copy link
Member

These are free functions, so I don't think there's as much of a concern with collisions as you'd have with new methods.

@scottmcm
Copy link
Member

Since the naming is uncertain and insta-stable is proposed anyway, could they just be

impl<'a, T> From<&'a T> for &'a [T] { ... }
impl<'a, T> From<&'a mut T> for &'a mut [T] { ... }

@whitequark
Copy link
Member Author

Personally, I think slice::from_ref is an obviously better name than slice::ref_slice. If I thought about naming at all (I did not, I just copied them from old rustc sources) then I would have chosen that too.

A From impl seems more disruptive to me.

@alexcrichton
Copy link
Member

I'd personally prefer to initially add these as unstable (but perhaps push on stabilization soon after landing), and I agree with @whitequark that a name like from_ref may be a bit nicer!

@aidanhs aidanhs removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Oct 20, 2017
@Ixrec
Copy link
Contributor

Ixrec commented Oct 23, 2017

Also strongly agree with from_ref. My first guess as to what slice::ref_slice would do was take a slice of Ts and convert it to a slice of refs of T.

@whitequark whitequark changed the title Bring back slice::ref_slice. Bring back slice::ref_slice as slice::from_ref. Oct 23, 2017
These functions were deprecated and removed in 1.5, but such simple
functionality shouldn't require using unsafe code, and it isn't
cluttering libstd too much.
@whitequark
Copy link
Member Author

Already renamed it, just not reflected in the PR.

@carols10cents
Copy link
Member

ping @BurntSushi waiting for your ticky box here!

@carols10cents carols10cents added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 30, 2017
@steveklabnik
Copy link
Member

My crate that provides these functions also provides an Option variant: https://github.com/steveklabnik/ref_slice/blob/master/src/lib.rs

I'm not sure if they're worth bringing in too, figured I'd bring it up.

@whitequark
Copy link
Member Author

@alexcrichton
Copy link
Member

I'm going to go ahead and check @BurntSushi's checkbox as this is just unstable anyway.

@rfcbot
Copy link

rfcbot commented Oct 31, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Oct 31, 2017
@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Oct 31, 2017
@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Oct 31, 2017

📌 Commit 8431811 has been approved by alexcrichton

@@ -2450,6 +2450,22 @@ pub unsafe fn from_raw_parts_mut<'a, T>(p: *mut T, len: usize) -> &'a mut [T] {
mem::transmute(Repr { data: p, len: len })
}

/// Converts a reference to T into a slice of length 1 (without copying).
#[stable(feature = "from_ref", since = "1.22.0")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we intend to insta-stable these?

@kennytm
Copy link
Member

kennytm commented Nov 1, 2017

@bors r-

If the final FCP checkbox expects "this is just unstable anyway" the current code certainly isn't unstable.

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Nov 1, 2017
@alexcrichton
Copy link
Member

Oops sorry, thanks for catching that @kennytm!

@whitequark
Copy link
Member Author

Can you please guide me on the stability markers here?

@kennytm
Copy link
Member

kennytm commented Nov 1, 2017

@whitequark

  1. File a tracking issue

  2. Replace those #[stable] attributes with

    #[unstable(feature = "from_ref", issue = "45678")]
  3. Write an unstable-book entry in src/doc/unstable-book/src/library-features/from-ref.md if you like.

@whitequark
Copy link
Member Author

Tracking issue filed as #45703, no unstable book entry since it seems far too trivial.

@whitequark
Copy link
Member Author

Stability annotation updated.

@kennytm
Copy link
Member

kennytm commented Nov 1, 2017

Thanks! It should be ready to go after fixing the build failure.

[00:02:34] error: use of unstable library feature 'from_ref' (see issue #45703)
[00:02:34]    --> /checkout/src/liballoc/slice.rs:123:23
[00:02:34]     |
[00:02:34] 123 | pub use core::slice::{from_ref, from_ref_mut};
[00:02:34]     |                       ^^^^^^^^
[00:02:34]     |
[00:02:34]     = help: add #![feature(from_ref)] to the crate attributes to enable
[00:02:34] 
[00:02:34] error: use of unstable library feature 'from_ref' (see issue #45703)
[00:02:34]    --> /checkout/src/liballoc/slice.rs:123:33
[00:02:34]     |
[00:02:34] 123 | pub use core::slice::{from_ref, from_ref_mut};
[00:02:34]     |                                 ^^^^^^^^^^^^
[00:02:34]     |
[00:02:34]     = help: add #![feature(from_ref)] to the crate attributes to enable

@whitequark
Copy link
Member Author

Updated.

@steveklabnik
Copy link
Member

steveklabnik commented Nov 1, 2017 via email

@kennytm
Copy link
Member

kennytm commented Nov 1, 2017

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Nov 1, 2017

📌 Commit 1cc88be has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Nov 2, 2017

⌛ Testing commit 1cc88be with merge d5b69d4...

bors added a commit that referenced this pull request Nov 2, 2017
Bring back slice::ref_slice as slice::from_ref.

These functions were deprecated and removed in 1.5, but such simple
functionality shouldn't require using unsafe code, and it isn't
cluttering libstd too much.

The original removal was quite contentious (see #27774), since then
we've had precedent for including such nuggets of functionality (see rust-lang/rfcs#1789),
and @nikomatsakis has provided a lot of use cases in rust-lang/rfcs#1789 (comment).
Hence this PR.

I'm not too sure what to do with stability, feel free to correct me.
It seems pointless to go through stabilization for these functions though.

cc @aturon
@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 2, 2017
@bors
Copy link
Contributor

bors commented Nov 2, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing d5b69d4 to master...

@bors bors merged commit 1cc88be into rust-lang:master Nov 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.